Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/fromager/bootstrapper.py`:
- Around line 1535-1541: The wheel mirror update is happening before the routed
cache entry is stored, which can leave the internal index out of sync for the
newly copied path. In `bootstrapper.py`, reorder the flow in the cache-hit
handling around `server.update_wheel_mirror`, `self.ctx.package_build_info`, and
`self.ctx.cache.store_wheel` so the wheel is stored in the routed collection
first and the mirror is updated afterward. Keep the existing routing logic
intact, just move the mirror refresh to run after `store_wheel()` completes.

In `@src/fromager/cache.py`:
- Around line 664-667: After backend.fetch in the cache hit path, register the
downloaded artifact with collection.store_backend so its index is updated as a
local artifact instead of remaining remote-only. Use the existing cache-hit flow
around backend.fetch and the collection.store_backend object to add the
downloaded local_path back into the store manager in the same way promoted
artifacts are tracked, so live cache/admin commands can see it.
- Around line 454-463: Verify and enforce the advertised sha256 when downloading
a remote wheel in fetch(). After the session.get/iter_content write completes,
compute the file’s SHA256 for the downloaded target and compare it against
info.sha256 from ArtifactInfo; if it does not match, reject the wheel by
removing the cached file and raising an error. Keep the existing download flow
in fetch() but add the hash validation before returning target so only verified
wheels are accepted.
- Around line 504-535: The _parse_project_page() helper is accepting untrusted
anchor text as ArtifactInfo.filename, which later gets joined by fetch() when
writing to dest, so reject any remote wheel name containing path components or
traversal segments. Update _parse_project_page() to validate the parsed filename
before appending ArtifactInfo, and only allow a plain basename for remote
artifacts while keeping the existing URL and sha256 parsing logic intact.

In `@src/fromager/commands/cache_cmd.py`:
- Around line 92-99: The variant cache setup in cache_cmd.py is reusing the same
prebuilt_backend instance in both the variant and default collections, which
breaks collection scoping and causes double counting/mutating shared files.
Update the CacheCollection construction logic around wkctx.variant so the
variant collection only owns variant-specific backends, while prebuilt_backend
remains attached to the default collection and is still reachable via
search_order. Keep the unique symbols CacheCollection, variant_backends,
collections[wkctx.variant], and search_order aligned so cache list/stats and
invalidate --collection <variant> operate on separate backend instances.

In `@tests/test_bootstrapper_iterative.py`:
- Around line 1319-1368: The cache hit stats test is bypassing the real lookup
path by mocking _find_cached_wheel_via_manager, so CacheManager.lookup_wheel()
never executes and the hit event cannot be verified. Update
test_cache_hit_records_stats to exercise the actual manager lookup path by only
stubbing the unpack step (for example, _unpack_metadata_from_wheel) and then
assert the relevant stats counter/event produced by CacheManager/Bootstrapper.
If this test is only duplicating phase progression, rename or remove it so it
validates the intended stats behavior.

---

Nitpick comments:
In `@tests/test_cache.py`:
- Around line 1045-1103: The remote-hit test currently only verifies the
download result and does not confirm same-run promotion into the local cache.
Update test_remote_hit_downloads_to_local_store in CacheManager flow to perform
a second lookup or directly query local_backend after the first remote hit, and
assert that the item is now served from LocalDirectoryBackend as a local hit.
Use the existing symbols CacheManager.lookup_wheel, local_backend, and
RemotePEP503Backend to keep the test focused on promotion behavior.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/fromager/cache.py`:
- Line 599: The assignment in the cache setup is treating an explicit empty
variant_packages set as missing, so callers can unintentionally fall back to
accelerated_packages. Update the logic in the relevant cache initialization path
around self._variant_packages to distinguish None from an empty set, preserving
an explicitly provided empty variant_packages while still using
accelerated_packages only when variant_packages is not supplied.
- Around line 686-692: The cache lookup path in lookup_wheel/cache fetch
handling treats a fetch failure as fatal instead of falling through to the next
cache or build fallback. Update the logic around backend.fetch and the local
index store in cache.py so exceptions from unusable hits (HTTP errors, hash
mismatch, I/O failures) are caught and treated like a miss, allowing the loop
over backends to continue. Keep the successful-path registration with
collection.store_backend.store intact, but only after a confirmed fetch.
- Around line 366-369: The cache write path in store() currently skips copying
when a same-named artifact already exists, which can leave stale wheels on disk
while still treating the new build as stored. Update the logic around
Cache.store() and the dest check so existing entries are either verified before
reuse or overwritten with the newly built artifact before indexing, ensuring the
cache index always matches the actual file on disk.
- Around line 461-474: The download path in the cache handling logic should not
trust an existing wheel at target without validating it first, and it should not
leave partial files behind after a failed or interrupted write. Update the
cache/download flow around the existing target check and the write-and-hash
block in the cache module so that an existing file is SHA256-verified before
returning it, and any checksum mismatch or write failure removes the target
before raising. Use the existing download helper and the hasher/target logic to
keep validation and cleanup consistent.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/fromager/cache.py`:
- Around line 475-489: The temporary download path in the cache download flow is
deterministic, so parallel fetches can overwrite each other’s in-progress file.
Update the download logic in the cache helper that creates tmp_target and later
calls tmp_target.replace(target) to use a unique temp file under dest for each
download, then keep the atomic replace into target once the content and sha256
check succeed. Ensure the change stays localized to the existing
download-and-verify path so concurrent builders do not share the same temporary
filename.
- Around line 367-369: The cache write in the artifact copy path is not atomic,
so concurrent readers can see a partially written wheel. Update the logic in the
copy block that uses `self._directory`, `dest`, and `shutil.copy2` to write to a
unique temporary file in the same directory first, then atomically swap it into
place with `replace()`. Keep the existing freshness check around
`artifact.samefile(dest)`/`dest.exists()` and apply the atomic replace only when
the cached file needs updating.

---

Nitpick comments:
In `@src/fromager/cache.py`:
- Around line 718-725: The cache hit warning in the requirement fetch path is
missing the structured per-requirement logging context. Update the logging in
the code path around the cache fetch warning to run inside req_ctxvar_context()
so the warning emitted from the requirement-specific logic carries the same
requirement context used elsewhere. Keep the existing logger.warning message and
symbols like req, version, collection_name, backend.name, and err, but ensure
the warning is emitted within the req_ctxvar_context() wrapper.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/fromager/cache.py`:
- Around line 324-335: Skip symlinked wheels during cache indexing in the logic
that scans self._directory in cache.py before building WheelCacheKey and
ArtifactInfo. Check each wheel_file from the glob and continue past any symlink
so url_or_path never points to a resolved target outside the cache root. Keep
the change localized to the wheel discovery loop used by invalidate/gc to
prevent later unlink operations from following symlinks.
- Around line 573-588: The URL handling in the artifact resolution logic still
allows plaintext http:// wheel links and accepts downloads without integrity
metadata. Update the URL resolution path in cache.py so the link parsing code
that builds url from href and strips sha256 fragments rejects any non-HTTPS
artifact URL or otherwise enforces that a valid sha256 is present before the
artifact is accepted. Make the check in the same flow that handles href
resolution and fragment parsing so absolute URLs cannot bypass the integrity
requirement.

In `@src/fromager/commands/cache_cmd.py`:
- Around line 357-398: Reject negative values for the gc command’s --keep-latest
option by constraining the Click parameter at the boundary in gc and/or the
`@click.option` definition. Update the keep_latest handling so only zero or
positive integers are accepted before the entries[keep_latest:] slicing logic
runs, preventing negative indexes from causing unintended cache deletions.

In `@tests/test_cache.py`:
- Around line 329-346: The test URLs in RemotePEP503Backend setup use
cache.example.com, but repository conventions require .test domains in tests.
Update the affected assertions and fixture-like instantiations in test_cache.py
to use cache.test instead, keeping the same RemotePEP503Backend cases such as
test_name_default, test_not_writable, and test_store_raises.

---

Nitpick comments:
In `@tests/test_cache.py`:
- Around line 1143-1155: The integration test for manager.lookup_wheel only
checks the downloaded wheel result and misses verifying that a remote hit is
recorded in the local backend index. After the existing assertions, add an
assertion using local_backend.lookup(key) to confirm
collection.store_backend.store() updated the local index for later lookups/admin
commands. Use the existing manager.lookup_wheel flow and the local_backend/key
symbols from this test to locate the right spot.
- Around line 566-592: The existing-file reuse test in
test_fetch_skips_existing_file only covers the no-hash case, so extend it to
include an ArtifactInfo with sha256 set and a preexisting corrupt file in the
destination. Update the assertions around RemotePEP503Backend.fetch so the
existing-file shortcut is only accepted when the file matches the expected
SHA256, and verify that a hash mismatch triggers re-download instead of
returning the stale path. Use the existing test_fetch_skips_existing_file,
RemotePEP503Backend.fetch, and ArtifactInfo symbols to keep the new case aligned
with the current behavior.